Skip to content

Allowed RedirectURIs fixed #168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
dmitry-krasilnikov wants to merge 1 commit into from
Closed

Allowed RedirectURIs fixed #168

dmitry-krasilnikov wants to merge 1 commit into from

Conversation

@dmitry-krasilnikov
Copy link

@dmitry-krasilnikov dmitry-krasilnikov commented Nov 18, 2014

  1. There is a problem with allowed redirect URIs check. Redirect URI must start with one of the allowed URIs, it does not have to be fully equal. It is generally accepted practice, for example Slack uses such approach when authorizes clients with OAuth2 protocol.
  2. The second problem is that redirect URIs can contain query strings after '?' symbol, some consumers could use query strings for some sort of reasons. It is better to compare paths without query strings.

I've stumbled on these when I was writing backend for django-social-auth which is used by Sentry.

@coveralls
Copy link

coveralls commented Nov 18, 2014

Coverage Status

Coverage increased (+0.0%) when pulling a506fda on dmitry-krasilnikov:master into 5ed4f50 on evonove:master.

@dmitry-krasilnikov
Copy link
Author

dmitry-krasilnikov commented Nov 27, 2014

Hi! I was just curious, have you seen my proposed changes? Thanks!

zopieux
zopieux reviewed Jan 8, 2015
View reviewed changes
oauth2_provider/models.py
Copy link
Contributor

@zopieux zopieux Jan 8, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might want to use the urlparse (or Py3 urllib.parse) to do this kind of extraction.

@zopieux zopieux mentioned this pull request Jan 13, 2015
Closed
@synasius
Copy link
Contributor

synasius commented Jan 15, 2015

This feature is added with the following commit: d17d3ea

@synasius synasius closed this Jan 15, 2015
@dmitry-krasilnikov
Copy link
Author

dmitry-krasilnikov commented Jan 16, 2015

Thanks! But what about Grant.redirect_uri_allowed method? It has exactly the same bug, because of full URI comparison including query string

@synasius
Copy link
Contributor

synasius commented Jan 16, 2015

That check is correct. It only applies to the Authorization Code Grant when the Access Token is requested. See http://tools.ietf.org/html/rfc6749#section-4.1.3

redirect_uri
REQUIRED, if the "redirect_uri" parameter was included in the
authorization request as described in Section 4.1.1, and their
values MUST be identical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dmitry-krasilnikov @coveralls @synasius @zopieux